Move Cassandra env variable setup to protected method#5991
Conversation
With testcontainers#2830 we added some environment variable setup to the constructor in CassandraContainer. This causes problems in scenarios where users customize the environment themselves or customize the values in cassandra.yaml, so move this init to a protected method to facilitate overriding.
eddumelendez
left a comment
eddumelendez
left a comment
There was a problem hiding this comment.
thanks again @akhaku ! Just left a comment to be reviewed by the team.
| withEnv("CASSANDRA_SNITCH", "GossipingPropertyFileSnitch"); | ||
| withEnv("JVM_OPTS", "-Dcassandra.skip_wait_for_gossip_to_settle=0 -Dcassandra.initial_token=0"); | ||
| withEnv("HEAP_NEWSIZE", "128M"); | ||
| withEnv("MAX_HEAP_SIZE", "1024M"); | ||
| withEnv("CASSANDRA_ENDPOINT_SNITCH", "GossipingPropertyFileSnitch"); | ||
| withEnv("CASSANDRA_DC", DEFAULT_LOCAL_DATACENTER); |
There was a problem hiding this comment.
IIRC those env were added in order to improve cassandra startup. I think we can apply those only if the image name is cassandra. @kiview WDYT?
It can be also applied for library/cassandra for those using a local registry. but we can do something like new CassandraContainer("library/cassandra:3.2.1").applyDefaultEnvVars(). Just throwing an idea
There was a problem hiding this comment.
Thanks, yeah no strong opinions on the mechanism apart from maybe not coupling it to the image name directly, seems a little brittle and might still cause problems for anyone who copies their own cassandra.yaml in into the container. Moving it to an overridable protected method was the least obtrusive way to allow turning it off but making it explicitly opt-in is fine too! I suppose we'll wait for Kevin to weigh in.
There was a problem hiding this comment.
@eddumelendez I think our container implementations should always assume a compatible image and not bind specific integration to the name. Else, it will become more of a hassle, once users provide their own image (also in this case we should assume a working image as default IMO).
@akhaku TBH, I don't understand the PR fully. Isn't it enough, if you do setEnv(new ArrayList() if you want to remove the defaults here?
There was a problem hiding this comment.
Ah thanks for that, I didn't realize that I could empty out the environment map like that. That workaround is fine by me!
Moving it to a protected method makes it easer to extend (e.g. I could call super.initEnv in my overridden method before or after adding my own env vars) but calling setEnv with an empty list works for my use case! I'll go ahead and decline this PR for now.
Also, only just realized that I forgot to add a call to initEnv in the constructor, I meant to have that there so it doesn't change existing behavior 🤦 sorry that probably caused some confusion too.
|
Closing this PR because as @kiview suggested, |
3 participants
With #2830 we added some environment variable setup to the constructor in CassandraContainer. This causes problems in scenarios where users customize the environment themselves or customize the values in cassandra.yaml, so move this init to a protected method to facilitate overriding.